xds: Changes to XdsClient Watcher APIs #12446

shivaspeaks · 2025-10-27T14:23:14Z

Implements https://github.com/grpc/proposal/blob/master/A88-xds-data-error-handling.md#a88-xds-data-error-handling

kannanjgithub

Sending what I could review today.

kannanjgithub · 2025-10-28T12:56:55Z

xds/src/main/java/io/grpc/xds/client/XdsClient.java

+     * Called to deliver a transient error that should not affect the watcher's use of any
+     * previously received resource.
     *
     * <p>Note that we expect that the implementer to:


I think this comment now applies to both methods since both methods take Status argument now.

We can remove it from the method's Javadoc as you have put in the class' Javadoc.

kannanjgithub · 2025-10-28T13:12:33Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

              type, resource);
-          respTimer = null;
          onAbsent(null, activeCpc.getServerInfo());
+          respTimer = null;


Why swap the order of these statements?

The original code was setting respTimer = null before calling onAbsent(), which caused the timeout context to be lost, resulting in a generic "does not exist" error.

The original for onAbsent method did not have a check for if (respTimer == null). It only looked at the serverInfo.resourceTimerIsTransientError() flag to decide between calling watcher.onError() or watcher.onResourceDoesNotExist(). It could not distinguish between a timeout and other "not found" conditions.

kannanjgithub · 2025-10-28T13:13:19Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

          logger.log(XdsLogLevel.WARNING, "No working fallback XDS Servers found from {0}",
-              activeCpClient.getServerInfo().target());
+              activeCpc.getServerInfo().target());
        }


The changes above here seem unnecessary?

kannanjgithub · 2025-10-28T13:38:37Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+          executor.execute(() -> {
            try {
-              notifyWatcher(watcher, data);
+              watcher.onResourceChanged(update); // Call the new method


Remove the comment as we are passing StatusOr argument.

kannanjgithub · 2025-10-28T14:09:18Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+            continue;
+          }
+          if (subscriber.hasResult()) {
+            subscriber.onError(status, null); // This will become an onAmbientError


As the handling to call either onDataChanged or onAmbientError happens in ResourceSubscriber.onError, why not simply call it? You are only calling if subscriber.hasResult() is true. What if when it is false? Previously the onError was getting called in that case but not now.

kannanjgithub

Review comments.

kannanjgithub · 2025-11-17T12:41:51Z

xds/src/main/java/io/grpc/xds/client/XdsClient.java

+     * Called to deliver a transient error that should not affect the watcher's use of any
+     * previously received resource.
     *
     * <p>Note that we expect that the implementer to:


We can remove it from the method's Javadoc as you have put in the class' Javadoc.

kannanjgithub · 2025-11-17T12:54:00Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

      if (!Objects.equals(oldData, data)) {
-        for (ResourceWatcher<T> watcher : watchers.keySet()) {
+        StatusOr<T> update = StatusOr.fromValue(data);
+        for (Map.Entry<ResourceWatcher<T>, Executor> entry : watchers.entrySet()) {


Why change from iterating the keySet to iterating the entrySet? There is nothing wrong with it but it seems like an unnecessary change.
Can keep the executor inlined like before as well.

kannanjgithub · 2025-11-17T13:49:57Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

+
+        Status status;
+        if (respTimer == null) {
+          status = Status.NOT_FOUND.withDescription("Resource " + resource + " does not exist");


For resource deletions, if fail_on_data_error is false, we should not be setting data = null; and should be calling onAmbientError(NOT_FOUND). lastError should also be set to NOT_FOUND so both the data and the last error are available for replay for new watchers.
Only for fail_on_data_error true we should drop the resource (data = null;) and call onResourceChanged.

kannanjgithub · 2025-11-17T13:56:00Z

xds/src/main/java/io/grpc/xds/client/XdsClientImpl.java

      for (Map<String, ResourceSubscriber<? extends ResourceUpdate>> subscriberMap :
          resourceSubscribers.values()) {
        for (ResourceSubscriber<? extends ResourceUpdate> subscriber : subscriberMap.values()) {
-          if (subscriber.hasResult() || !authoritiesForClosedCpc.contains(subscriber.authority)) {


Why remove subscriber.hasResult()?

kannanjgithub · 2025-11-17T13:56:44Z

xds/src/main/java/io/grpc/xds/XdsDependencyManager.java

    private StatusOr<T> data;
+    @Nullable
+    @SuppressWarnings("unused")
+    private Status ambientError;


Will there be a separate PR for propagating this to the data plane rpc error?

kannanjgithub · 2025-11-17T13:59:45Z

xds/src/main/java/io/grpc/xds/XdsServerWrapper.java

      }
+
      if (!pendingRds.isEmpty()) {
-        // filter chain state has not yet been applied to filterChainSelectorManager and there


Restore this comment.

shivaspeaks and others added 20 commits August 11, 2025 20:09

MAINTAINERS.md: Add Shiva

21352b7

Merge branch 'grpc:master' into master

458735d

Update XdsTrustManagerFactoryTest.java

4f064fa

refactor and add documentation

0f96110

Merge branch 'master' of https://github.com/grpc/grpc-java

d7ef02a

Merge branch 'grpc:master' into master

405217b

Merge branch 'grpc:master' into master

587969d

xds: client watcher API changes

b1d0d6d

xds: client watcher API changes

3fcff8d

xds: client watcher API changes

bf905c2

xds: client watcher API changes

4f5f3b3

xds: client watcher API changes

494a75a

xds: client watcher API changes

a5117ad

xds: client watcher API changes

352ec9e

xds: client watcher API changes

148f105

xds: client watcher API changes

0c78954

xds: client watcher API changes

b5571d1

watcher api changes

c3a43b2

watcher api changes

910b20b

Merge pull request #3 from shivaspeaks/client-watcher-changes-2

319c113

shivaspeaks requested review from ejona86 and kannanjgithub October 27, 2025 14:23

shivaspeaks added 3 commits October 27, 2025 21:39

watcher api changes

2baf738

Merge branch 'master' of https://github.com/shivaspeaks/grpc-java

a51ac28

Merge branch 'client-watcher-changes-2' of https://github.com/shivasp…

3779aea

…eaks/grpc-java

shivaspeaks mentioned this pull request Oct 28, 2025

xds: client watcher API update #12046

Closed

kannanjgithub reviewed Oct 28, 2025

View reviewed changes

address comments

4ba6de4

kannanjgithub reviewed Nov 17, 2025

View reviewed changes

xds: Changes to XdsClient Watcher APIs #12446

Are you sure you want to change the base?

xds: Changes to XdsClient Watcher APIs #12446

Conversation

shivaspeaks commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kannanjgithub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shivaspeaks commented Oct 27, 2025 •

edited

Loading